Codecov Report

Merging #775 (dfdfe31) into main (7b38b52) will not change coverage.
The diff coverage is 100.00%.

❗ Current head dfdfe31 differs from pull request most recent head e5e4cd5. Consider uploading reports for the commit e5e4cd5 to get more accurate results

@@            Coverage Diff            @@
##              main      #775   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           49        49           
  Lines         1971      1971           
=========================================
  Hits          1971      1971           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

This should now be mostly functional, there are definitely some breaking changes in here though. I'd love for folks to take a look around and see if there's anything to improve ☺️

One suggestion would be to potentially define either a protocol or just a type union e.g. ClientT = Union[Client, AuthenticatedClient] so that type hinting of "clients" is a bit cleaner, especially if for some reason we add a third client type down the line. May be overkill but would be aesthetically pleasing haha

I think we're going to move to dynamically generating clients based on any security schemes defined (rather than the generic, often incorrect, AuthenticatedClient of today), and then allowing a union of just the applicable clients to each endpoint.

This probably requires some more thought, though, to get the interfaces right 🤔

Going to give this a week to marinate in case any of the authors of various issues I've mentioned this in want to give feedback.

Thanks for all the work getting this off the ground, but I have one concern from reading the changelog which I may have not understood correctly.
This looks like a drastic breaking change, no? We don't use "client" with a context manager, we wrap it in our own client which then reuses it. What's the recommended way for us to do cleanup? Does this mean we are now leaking resources unless we do something? Would really appreciate your guidance on this!

@tasn it really depends on what you're doing 😅. For long-lived clients (i.e., you're reusing the client for many requests), this change will be much more efficient by default as it will reuse TCP connections via connection pooling.

For short-lived processes (e.g., a Python script that runs for a few seconds or minutes and then ends), it doesn't matter much if you're not cleaning up, because when the process exits, the connections will be closed.

So, while best practice is to close connections either via a context manager or by manually calling .close() or .aclose() on the underlying httpx client, in reality it usually isn't an issue.

So, when do you need to worry (and thus, why did I put warnings in the changelog)? If you have a long lived Python process which creates many clients, each new client will create a new TCP connection. Eventually, if these are not cleaned up, you will run out of available ports on your underlying operating system. Garbage collection doesn't seem to help here, so this could be a big problem. Imagine you have a web server where, for each incoming request, it creates a new Client to call some downstream resource. You're going to have a limited number of requests you can handle before the whole thing stops working 😓.

So to recap, if you're already keeping a single Client instance around for all the requests you're making to a single API through the lifetime of the process, worry not! If you're recreating Clients at multiple times throughout the life of the process, either:

1. Switch to a single, global client (and test to make sure you don't hit multi-threading issues, I have no idea how httpx performs in a multi-threaded environment)
2. Start closing the clients when you're done with them, either via context managers (preferred) or a try/finally/close.